Skip to content

[JTC] Renaming variables, reordering trajectory checks#1032

Merged
christophfroehlich merged 10 commits intomasterfrom
cleaning-jtc
Feb 16, 2025
Merged

[JTC] Renaming variables, reordering trajectory checks#1032
christophfroehlich merged 10 commits intomasterfrom
cleaning-jtc

Conversation

@destogl
Copy link
Member

@destogl destogl commented Feb 9, 2024 •
edited by christophfroehlich
Loading

This PR increseas readability of the JTC by renaming trajectory varialbes to be semantically correct and self explainable.

Also it reoders checks of the trajectories done in the callback. They are grouped based on the things they are checking, i.e., 1. data available, 2. time checks; 3. trajectory point checks.

Additionally, it adds "last_commanded_time_" for the open loop mode to be more correct when setting point before trajectory. #780

@destogl destogl self-assigned this Feb 9, 2024
@mergify
Copy link
Contributor

mergify bot commented Feb 10, 2024

This pull request is in conflict. Could you fix it @destogl?

@bmagyar
Copy link
Member

bmagyar commented Feb 11, 2024

Recently merged the wraparound feature which caused some conflicts.

Also, this may be interesting for @fmauch

@bmagyar bmagyar changed the title [JTC] Renaming varialbes, reodering trajectory checks and fixing open-loop mode by adding last commanded time. [JTC] Renaming variables, reordering trajectory checks and fixing open-loop mode by adding last commanded time. Feb 11, 2024
christophfroehlich
christophfroehlich approved these changes Feb 12, 2024
View reviewed changes
Copy link
Member

@christophfroehlich christophfroehlich left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM and improves readability!

@christophfroehlich
Copy link
Member

christophfroehlich commented Feb 12, 2024

👀 @destogl test_no_jump_when_state_tracking_error_not_updated fails now, I guess due to the last_commanded_time_ change.

saikishor
saikishor previously approved these changes Feb 12, 2024
View reviewed changes
Copy link
Member

@saikishor saikishor left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, apart from the following minor comment.

Now with the changes, it looks clear and good

bmagyar
bmagyar previously approved these changes Feb 19, 2024
View reviewed changes
@bmagyar
Copy link
Member

bmagyar commented Feb 19, 2024

Please adjust / extend the tests

henrygerardmoore pushed a commit to henrygerardmoore/ros2_controllers that referenced this pull request Jul 19, 2024
@dependabot
Bump codecov/codecov-action from 3.1.3 to 3.1.4 (ros-controls#1032)
64d4709
@christophfroehlich christophfroehlich added backport-humble Triggers PR backport to ROS 2 humble. backport-iron labels Nov 2, 2024
christophfroehlich
christophfroehlich requested changes Nov 2, 2024
View reviewed changes
Copy link
Member

@christophfroehlich christophfroehlich left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This includes #780, which has the same failing test: test_no_jump_when_state_tracking_error_not_updated

@christophfroehlich christophfroehlich marked this pull request as draft November 2, 2024 20:20
@mergify
Copy link
Contributor

mergify bot commented Dec 16, 2024

This pull request is in conflict. Could you fix it @destogl?

@christophfroehlich christophfroehlich removed the backport-humble Triggers PR backport to ROS 2 humble. label Feb 14, 2025
@christophfroehlich christophfroehlich changed the title [JTC] Renaming variables, reordering trajectory checks and fixing open-loop mode by adding last commanded time. [JTC] Renaming variables, reordering trajectory checks Feb 14, 2025
@christophfroehlich christophfroehlich marked this pull request as ready for review February 14, 2025 22:25
@christophfroehlich christophfroehlich requested review from bmagyar, fmauch and saikishor and removed request for VanshGehlot February 14, 2025 22:25
@codecov
Copy link

codecov bot commented Feb 14, 2025 •
edited
Loading

Codecov Report

Attention: Patch coverage is 87.17949% with 5 lines in your changes missing coverage. Please review.

Project coverage is 84.75%. Comparing base (a048212) to head (f5b82cf).
Report is 2 commits behind head on master.

Files with missing lines Patch % Lines
...ory_controller/src/joint_trajectory_controller.cpp 86.84% 2 Missing and 3 partials ⚠️
Additional details and impacted files 
@@            Coverage Diff             @@
##           master    #1032      +/-   ##
==========================================
- Coverage   84.76%   84.75%   -0.01%     
==========================================
  Files         124      124              
  Lines       11444    11443       -1     
  Branches      967      967              
==========================================
- Hits         9700     9699       -1     
  Misses       1432     1432              
  Partials      312      312
Flag Coverage Δ
unittests 84.75% <87.17%> (-0.01%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines Coverage Δ
...jectory_controller/joint_trajectory_controller.hpp 100.00% <ø> (ø)
...ntroller/test/test_trajectory_controller_utils.hpp 83.92% <100.00%> (ø)
...ory_controller/src/joint_trajectory_controller.cpp 85.11% <86.84%> (-0.03%) ⬇️

... and 2 files with indirect coverage changes

saikishor
saikishor previously approved these changes Feb 15, 2025
View reviewed changes
Copy link
Member

@saikishor saikishor left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

in general, LGTM
2 minor comments, it is also not a problem not addressing them

@christophfroehlich @saikishor
Apply suggestions from code review
f5b82cf 
Co-authored-by: Sai Kishor Kothakota <sai.kishor@pal-robotics.com>
saikishor
saikishor approved these changes Feb 16, 2025
View reviewed changes
Copy link
Member

@saikishor saikishor left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@saikishor
Copy link
Member

saikishor commented Feb 16, 2025

@christophfroehlich it seems the downstream builds are failing from the UR drivers scaled JTC. Shall we inform Felix?

@christophfroehlich
Copy link
Member

christophfroehlich commented Feb 16, 2025

@christophfroehlich it seems the downstream builds are failing from the UR drivers scaled JTC. Shall we inform Felix?

The changes are simple, I can open a PR for the ur_controllers

@christophfroehlich christophfroehlich mentioned this pull request Feb 16, 2025
Merged
@christophfroehlich christophfroehlich merged commit fc56126 into master Feb 16, 2025
17 of 25 checks passed
@christophfroehlich christophfroehlich deleted the cleaning-jtc branch February 16, 2025 15:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@christophfroehlich christophfroehlich christophfroehlich approved these changes

@saikishor saikishor saikishor approved these changes

@bijoua29 bijoua29 Awaiting requested review from bijoua29

@VX792 VX792 Awaiting requested review from VX792

@bmagyar bmagyar Awaiting requested review from bmagyar

@fmauch fmauch Awaiting requested review from fmauch

@anfemosa anfemosa Awaiting requested review from anfemosa

@ARK3r ARK3r Awaiting requested review from ARK3r

@erickisos erickisos Awaiting requested review from erickisos

@mcbed mcbed Awaiting requested review from mcbed

@peterdavidfagan peterdavidfagan Awaiting requested review from peterdavidfagan

Assignees

@destogl destogl

Labels

enhancement

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@destogl @bmagyar @christophfroehlich @saikishor

Comments